feat: pkg.pr.new on main by Aslemammad · Pull Request #354 · vitejs/vite-ecosystem-ci

Aslemammad · 2025-01-19T18:18:50Z

Workflow runs for the main branch do not use the pkg.pr.new build, meanwhile the main branch of the vite repo is the place where each commit gets its own pkg.pr.new build which feels a bit wasted.

So this will try to look for pkg.pr.new builds for the main branch.

sapphi-red · 2025-01-24T03:33:29Z

@Aslemammad FYI the CI is failing

Aslemammad · 2025-01-24T06:17:23Z

Yes I will tackle it soon.

Aslemammad · 2025-01-26T13:40:29Z

tests/_selftest.ts

-			pkg.scripts.selftestscript =
-				"[ -d ../../vite/packages/vite/dist ] || (echo 'vite build failed' && exit 1)"
+			if (options.release?.startsWith('https://pkg.pr.new/vite@')) {
+				pkg.scripts.selftestscript =


@sapphi-red would love to hear your thoughts on this now that the ci is fixed by this!

sapphi-red · 2025-01-29T05:57:07Z

utils.ts

-	cd(vitePath)
+export async function getPermanentRef(repo: string, ref: string) {
 	try {
-		const ref = await $`git log -1 --pretty=format:%H`
-		return ref
+		const res = await fetch(
+			`https://api.github.com/repos/${repo}/branches/${ref}`,
+		)
+		const {
+			commit: { sha },
+		} = (await res.json()) as { commit: { sha: string } }
+		return sha


I don't think this is correct. What we want for the usage in discord-webhook.ts is the commit hash that the ecosystem-ci ran on. This is getting the latest commit hash when the ci finished (i.e. a different hash will be output when a new commit was pushed while the CI is running). (It is fine for the case of ecosystem-ci.ts as it's called before running.)

this should always use the local checkout if it exists and as mentioned by sapphi we can't have ambiguity introduced by new commits. So in case where no local checkout exists, you would have to obtain the ref in a deterministic way. Currently not sure what that is other than asking for it to be passed in (ie never just a branch name but always branch+revision)

sapphi-red · 2025-01-29T06:00:20Z

ecosystem-ci.ts

+		if (
+			options.branch === 'main' &&
+			options.repo === 'vitejs/vite' &&
+			!options.commit
+		) {
+			const sha = await getPermanentRef(options.repo, options.branch)
+			if (sha) {
+				options.commit = sha
+			}
+		}
 		if (options.commit) {
 			const url = `https://pkg.pr.new/vite@${options.commit}`
-			//eslint-disable-next-line n/no-unsupported-features/node-builtins
 			const { status } = await fetch(url)
 			if (status === 200) {
 				options.release = url


By this code, we will sometimes get a prebuilt package and sometimes don't, depending on when the last commit was pushed. I think we should avoid that if possible so that there aren't differences among the runs.

i see, can you show me an instance where we give the prebuilt package? i'd like to see how can i manipulate the code to perform well on those edge-cases.

i wonder if putting if (options.commit && !options.release) would resolve that edge case or no? would love to hear your thoughts.

The example case of using a prebuilt package is:

I push a commit in the main branch at 10:30.

The CI in Vite repo build the package and uploads it at 10:32.

The ecosystem-ci triggers at 10:35.

The ecosystem-ci uses the prebuilt package.

The example case of not using a prebuilt package is:

I push a commit in the main branch at 10:30.

The ecosystem-ci triggers at 10:31.

The ecosystem-ci does not use a prebuilt package.

The CI in Vite repo build the package and uploads it at 10:32.

i wonder if putting if (options.commit && !options.release) would resolve that edge case or no?

I think that doesn't resolve the case.

tests/_selftest.ts

Aslemammad · 2025-01-29T08:01:29Z

will resolve the comments asap!

Co-authored-by: 翠 / green <green@sapphi.red>

dominikg · 2025-03-03T15:19:17Z

ecosystem-ci.ts

+			let attempts = 0
+			do {
+				const { status } = await fetch(url)
+				if (status !== 200) {


why is this !== instead of the previous === ?

hey this is for debugging and i need to make few changes.

dominikg · 2025-03-03T15:21:14Z

ecosystem-ci.ts

-
-				console.log(`continuous release available on ${url}`)
-			}
+			const maxAttempts = 60 // 5 minutes


currently vite-ecosystem-ci takes less than one minute to build vite from source. I don't think polling 5 minutes to wait for it to maybe appear on pr.new is the right way to go here.

Either make sure the ecosystem-ci run is only triggered after pr.new is available or skip ahead and build it here.

I think it's important to make it consistent across commits, so I'd prefer not to skip ahead. How about throwing an error if it exceeds 1 minutes? The built package should be available at that time.

i don't think ecosystem-ci should be reliant on pr.new availability so fallback is needed.

I'd prefer the wait/poll was implemented at the trigger side, not here

I'd prefer the wait/poll was implemented at the trigger side, not here

The trigger is here in this repo.

vite-ecosystem-ci/.github/workflows/ecosystem-ci.yml

Lines 15 to 17 in 66423f9

on:

schedule:

- cron: "0 5 * * 1,3,5" # monday,wednesday,friday 5AM

it should still go into that job then but not convinced we need this. what's the advantage of pr.new on scheduled runs?

hey! sorry for the long wait, it's been a bit hard on my side.

it has the same benefits that it has on triggered runs from PRs! it'd avoid the building part on each scheduled run since there's a high chance that commit was already built.

And if it was not built, we wait a bit until it'd be.

But anyway, let me know, I'm ok with making either polling or falling back work.

building vite here takes 14 seconds (thanks rolldown!)

is it really going to be faster to check pr.new? Esp. the polling variant feels like overkill. If its there and usable immediately, ok. but else just do it here. scheduled runs (and PR runs too) are unlikely to hit the same commit hash twice, so unless it is cached already there is no benefit for ecosystem-ci

I'd like to keep the behavior consistent across the runs, otherwise there'll be problems that are difficult to investigate like vitejs/vite#20080 (comment).
If the polling is overkill, I think we should always built it here.

Aslemammad added 5 commits January 19, 2025 21:47

init

042e65a

dry

06852c3

get the comment back

00a6c5e

update

505fe41

update

8e0cb06

fix ci

627a5a9

Aslemammad commented Jan 26, 2025

View reviewed changes

sapphi-red reviewed Jan 29, 2025

View reviewed changes

Aslemammad and others added 9 commits January 29, 2025 18:50

Update tests/_selftest.ts

a56f13c

Co-authored-by: 翠 / green <green@sapphi.red>

Update tests/_selftest.ts

f76ef85

Co-authored-by: 翠 / green <green@sapphi.red>

no getPermanentRef for discord

896ce89

some updates

04ec121

pass COMMIT to discord-webhook.ts

ade5945

update

faab52e

update

257b0ee

format

1856507

polling on main

603cee8

dominikg reviewed Mar 3, 2025

View reviewed changes

throw after 1 minute of polling

2d0e8ed

	on:
	schedule:
	- cron: "0 5 * * 1,3,5" # monday,wednesday,friday 5AM

Uh oh!

Conversation

Aslemammad commented Jan 19, 2025

Uh oh!

sapphi-red commented Jan 24, 2025

Uh oh!

Aslemammad commented Jan 24, 2025

Uh oh!

Aslemammad Jan 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Aslemammad commented Jan 29, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Aslemammad Jan 26, 2025 •

edited

Loading